-
Notifications
You must be signed in to change notification settings - Fork 70
support fixed gas fee cap and tip cap #868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional fixed gas fee and tip caps (CLI flags → config → L2Client). L2Client stores caps and exposes accessors; signing and tx-manager logic compute dynamic tip/base-fee, clamp values to configured caps when present, and apply resulting GasTipCap/GasFeeCap before estimation and sending. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant L2Client
participant EthNode as EthereumNode
participant TxManager
participant Signer
Caller->>L2Client: GetOpts()
alt Fixed caps configured
L2Client->>L2Client: Return opts with GasTipCap & GasFeeCap (fixed)
else Dynamic caps
L2Client->>EthNode: SuggestGasTipCap()
EthNode-->>L2Client: tip
L2Client->>EthNode: HeaderByNumber("latest")
EthNode-->>L2Client: head (BaseFee?)
alt head.BaseFee present
L2Client->>L2Client: fee = tip + 2*BaseFee
else
L2Client->>L2Client: fee = tip
end
L2Client->>Caller: return opts with dynamic caps
end
Caller->>TxManager: sendWithLocalSign(auth)
TxManager->>TxManager: applyGasCaps(auth) — suggest tip, fetch header, clamp to configured caps
TxManager->>Signer: EstimateGas(auth with GasTipCap/GasFeeCap) + Sign
Signer->>EthNode: SendRawTransaction
EthNode-->>Signer: TxHash
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@token-price-oracle/client/l2_client.go`:
- Around line 55-63: The code currently converts uint64 cfg.GasFeeCap and
cfg.GasTipCap to signed int64 via big.NewInt(int64(...)) which can overflow;
update the assignment for l2Client.gasFeeCap and l2Client.gasTipCap to construct
unsigned big.Ints (use new(big.Int).SetUint64(cfg.GasFeeCap) and
new(big.Int).SetUint64(cfg.GasTipCap)) and keep the existing log.Info calls (log
the original uint64 values) so the gas caps are correctly represented as
unsigned big.Ints rather than risking negative values from int64 overflow.
🧹 Nitpick comments (3)
token-price-oracle/config/config.go (1)
88-91: Consider validating thatGasFeeCap >= GasTipCapwhen both are set.EIP-1559 requires
gasFeeCap >= gasTipCap. When both fixed values are provided, an invalid combination (e.g.,GasTipCap > GasFeeCap) would produce a transaction that gets rejected at submission time with a potentially confusing error. A quick check inLoadConfigwould surface this early.Suggested validation in LoadConfig (after line 120)
// Add after loading GasFeeCap and GasTipCap (around line 121) if cfg.GasFeeCap > 0 && cfg.GasTipCap > 0 && cfg.GasTipCap > cfg.GasFeeCap { return nil, fmt.Errorf("gas tip cap (%d) must not exceed gas fee cap (%d)", cfg.GasTipCap, cfg.GasFeeCap) }token-price-oracle/client/l2_client.go (2)
156-162: Shared*big.Intreference assigned toopts— callers could inadvertently mutate internal state.
c.gasFeeCapis assigned directly toopts.GasFeeCapwithout copying. If any downstream code (e.g., thebindpackage or gas estimation) mutates the*big.Intin-place, it would corrupt the fixed cap for all subsequent transactions. This is a defensive concern given current callers don't appear to mutate, but worth protecting.Suggested defensive copy
if c.gasFeeCap != nil { - opts.GasFeeCap = c.gasFeeCap + opts.GasFeeCap = new(big.Int).Set(c.gasFeeCap) } if c.gasTipCap != nil { - opts.GasTipCap = c.gasTipCap + opts.GasTipCap = new(big.Int).Set(c.gasTipCap) }
190-199: Same shared-pointer concern applies to the accessors.
GetFixedGasFeeCap()andGetFixedGasTipCap()return the internal*big.Intdirectly. Ifsign.go(or a future caller) mutates the returned value, it corrupts the L2Client's state. Returning a copy would be safer.Suggested defensive copy
func (c *L2Client) GetFixedGasFeeCap() *big.Int { + if c.gasFeeCap == nil { + return nil + } - return c.gasFeeCap + return new(big.Int).Set(c.gasFeeCap) } func (c *L2Client) GetFixedGasTipCap() *big.Int { + if c.gasTipCap == nil { + return nil + } - return c.gasTipCap + return new(big.Int).Set(c.gasTipCap) }
…e min of dynamic and configured value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
token-price-oracle/client/sign.go (1)
99-134:⚠️ Potential issue | 🔴 CriticalClamping tip and fee cap independently can violate the EIP-1559 invariant
gasFeeCap >= gasTipCap.Tip is clamped first (lines 104-108), then
gasFeeCapis computed from clamped tip and later independently clamped (lines 129-133). If onlygas-fee-capis configured (withoutgas-tip-cap), a high dynamic tip can exceed the clampedgasFeeCap, producing an invalid transaction.Example: dynamic tip = 100 gwei, baseFee = 50 gwei → computed gasFeeCap = 200 gwei. If
gas-fee-capis set to 80 gwei butgas-tip-capis unset →gasFeeCap = 80,tip = 100→ gasFeeCap < tip, transaction rejected.Add a post-clamping guard to enforce the invariant:
Proposed fix — clamp tip to gasFeeCap after all caps are applied
// Apply gas fee cap limit if configured if maxFeeCap := client.GetFixedGasFeeCap(); maxFeeCap != nil { if gasFeeCap.Cmp(maxFeeCap) > 0 { log.Debug("Applying gas fee cap limit", "dynamic", gasFeeCap, "cap", maxFeeCap) gasFeeCap = maxFeeCap } } + + // Ensure EIP-1559 invariant: gasFeeCap >= gasTipCap + if gasFeeCap.Cmp(tip) < 0 { + log.Warn("Gas fee cap is lower than tip cap, clamping tip to fee cap", "gasFeeCap", gasFeeCap, "tip", tip) + tip = new(big.Int).Set(gasFeeCap) + }
🤖 Fix all issues with AI agents
In `@token-price-oracle/config/config.go`:
- Around line 119-127: When reading flags in config.go for flags.GasFeeCapFlag
and flags.GasTipCapFlag, validate and reject zero values and enforce GasFeeCap
>= GasTipCap: when ctx.IsSet(flags.GasFeeCapFlag.Name) or
ctx.IsSet(flags.GasTipCapFlag.Name) read the uint64 into a local v, return an
error if v == 0 (do not assign to cfg.GasFeeCap/cfg.GasTipCap), and only assign
the pointers after validation; if both caps are provided, check that
cfg.GasFeeCap >= cfg.GasTipCap and return an error if not (this prevents the
invalid state that breaks the EIP-1559 invariant and avoids the clamping issue
in sign.go).
🧹 Nitpick comments (1)
token-price-oracle/client/sign.go (1)
104-108: Potential aliasing with internal client state in gas cap assignments.The
GetFixedGasTipCap()andGetFixedGasFeeCap()methods return direct references to the client's internal*big.Intfields (gasTipCapandgasFeeCap). This creates aliasing when assigned viatip = maxTip(line 104) andgasFeeCap = maxFeeCap(line 131), where local variables reference the client's state.While the current code only reads these variables after assignment (no in-place mutations), this pattern is fragile and may lead to subtle bugs if future changes add arithmetic operations on
tiporgasFeeCapafter these assignments.Use defensive copies to prevent aliasing:
Line 104
if maxTip := client.GetFixedGasTipCap(); maxTip != nil { if tip.Cmp(maxTip) > 0 { log.Debug("Applying gas tip cap limit", "dynamic", tip, "cap", maxTip) - tip = maxTip + tip = new(big.Int).Set(maxTip) }Line 131
if maxFeeCap := client.GetFixedGasFeeCap(); maxFeeCap != nil { if gasFeeCap.Cmp(maxFeeCap) > 0 { log.Debug("Applying gas fee cap limit", "dynamic", gasFeeCap, "cap", maxFeeCap) - gasFeeCap = maxFeeCap + gasFeeCap = new(big.Int).Set(maxFeeCap) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@token-price-oracle/client/sign.go`:
- Around line 104-109: The code assigns tip = maxTip which aliases L2Client's
internal *big.Int and risks shared-state corruption; in the block where
client.GetMaxGasTipCap() is non-nil and tip.Cmp(maxTip) > 0, replace the direct
assignment with a defensive copy (e.g., tip = new(big.Int).Set(maxTip)) so tip
no longer points to the client-owned gasTipCap; update the logic in sign.go
where tip is compared/assigned to use the copy.
- Around line 129-134: The gasFeeCap assignment currently aliases the pointer
(gasFeeCap = maxFeeCap) and doesn't ensure gasTipCap ≤ gasFeeCap; change to copy
the value from client.GetMaxGasFeeCap() into a new big.Int (so gasFeeCap is
independent) and after clamping gasFeeCap, enforce the invariant by clamping
gasTipCap (or tip) to be ≤ gasFeeCap (i.e., if gasTipCap.Cmp(gasFeeCap) > 0 then
set gasTipCap to a copy of gasFeeCap); apply the same pointer-copy +
tip-clamping logic in tx_manager.go's applyGasCaps to avoid creating invalid
EIP‑1559 transactions (reference symbols: gasFeeCap, maxFeeCap, gasTipCap, tip,
client.GetMaxGasFeeCap(), applyGasCaps).
In `@token-price-oracle/updater/tx_manager.go`:
- Around line 202-252: The applyGasCaps function currently can leave tip >
gasFeeCap after clamping maxFeeCap; after you set gasFeeCap (and after the
maxFeeCap clamp) add the same post-clamp invariant enforcement as in sign.go:
ensure tip ≤ gasFeeCap by comparing tip (or auth.GasTipCap) with gasFeeCap and,
if tip.Cmp(gasFeeCap) > 0, set tip = new(big.Int).Set(gasFeeCap) and update
auth.GasTipCap accordingly before assigning auth.GasFeeCap and returning;
reference applyGasCaps, tip, gasFeeCap, maxFeeCap, auth.GasTipCap and
auth.GasFeeCap when implementing.
🧹 Nitpick comments (2)
token-price-oracle/client/l2_client.go (1)
183-191: Accessors return internal*big.Int— callers can mutate shared state.
GetMaxGasFeeCap()andGetMaxGasTipCap()return the internal pointer directly. Any caller that performs in-place operations (e.g.,result.Mul(...)) would silently corruptL2Client's state.tx_manager.gocopies before use, butsign.godoes not (see separate comment there). A defensive copy here would eliminate the class of bugs entirely.♻️ Suggested defensive copy
// GetMaxGasFeeCap returns the max gas fee cap (nil if not configured) func (c *L2Client) GetMaxGasFeeCap() *big.Int { + if c.gasFeeCap == nil { + return nil + } - return c.gasFeeCap + return new(big.Int).Set(c.gasFeeCap) } // GetMaxGasTipCap returns the max gas tip cap (nil if not configured) func (c *L2Client) GetMaxGasTipCap() *big.Int { + if c.gasTipCap == nil { + return nil + } - return c.gasTipCap + return new(big.Int).Set(c.gasTipCap) }token-price-oracle/client/sign.go (1)
85-172: Gas cap logic is duplicated betweenCreateAndSignTxandTxManager.applyGasCaps.The tip-fetch → clamp → header-fetch → feeCap-compute → clamp sequence is nearly identical in both
sign.go:CreateAndSignTx(lines 99-134) andtx_manager.go:applyGasCaps(lines 204-251). This duplication means any fix (e.g., thegasTipCap ≤ gasFeeCapinvariant) must be applied in two places. Consider extracting a shared helper, e.g.,computeGasCaps(ctx, client) (tip, feeCap *big.Int, err error), and calling it from both paths.
f27b70e to
6796e57
Compare
- Add client/gas.go with CalculateGasCaps and CalculateGasCapsAlways - Simplify tx_manager.go applyGasCaps to use shared function - Simplify sign.go CreateAndSignTx to use shared function - Ensures consistent gas cap strategy and EIP-1559 invariant handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@token-price-oracle/client/gas.go`:
- Around line 17-28: The CalculateGasCaps implementation currently always
returns caps despite the comment; change it so that if both
client.GetMaxGasTipCap() and client.GetMaxGasFeeCap() indicate "no configured
max" (e.g., nil or zero/unset value returned by those accessors)
CalculateGasCaps returns (nil, nil) immediately to honor its contract, otherwise
call doCalculateGasCaps(ctx, client, maxTipCap, maxFeeCap) as now; ensure
applyGasCaps and sign.go continue to handle a nil return path as documented.
…guration
Summary by CodeRabbit
Release Notes
New Features
gas-fee-capandgas-tip-capconfiguration flags to set maximum transaction gas costs via CLI or environment variablesImprovements